Fix dropped errors in manifold.time/in
#247
Conversation
DerGuteMoritz
changed the title
manifold.time/in
The result deferred `d` may end up in an error state when `(f)` throws an exception or returns an error deferred. Now since the cancellation callback was only attached via `d/chain` for side-effect (i.e. the resulting deferred is not returned), the dropped error detection would be triggered in this case. The fix is simply to also attach an error handler with `d/on-realized`. Furthermore, putting the result deferred in an error state would not have cancelled the timer because there was no error handler attached to it. This is now also fixed and the tests are extended to cover the cancellation API, too. The latter uncovered an oversight in the mock clock implementation where the `in` method didn't return a cancellation function, resulting in an NPE. This is now also fixed.
DerGuteMoritz
force-pushed
the
fix-dropped-errors-in-manifold-time
branch
from
febc716 to
69e53a7
Compare
| (in [this interval-millis f] | ||
| (swap! events update-in [(p/+ ^double @now interval-millis)] #(conj (or % []) f)) | ||
| (advance this 0)) | ||
| (let [t (p/+ ^double @now interval-millis) |
There was a problem hiding this comment.
shouldn't this be a swap! ?
There was a problem hiding this comment.
On now you mean? If so: no, the current clock time can only be changed via advance for the mock clock. Also note that this was already the case before - I merely pulled out the key construction from the update-in so that I can re-use it in cancel-fn.
| (update t disj f))))] | ||
| (swap! events update t (fnil conj #{}) f) | ||
| (advance this 0) | ||
| cancel-fn)) |
There was a problem hiding this comment.
this changes the return value from nil (what advance returns) to a fn.
is that ok?
There was a problem hiding this comment.
It's not only OK but fixes the mock clock implementation to conform to the expected return value. Quoting from the commit message / PR description:
The latter uncovered an oversight in the mock clock implementation where the
inmethod didn't return a cancellation function, resulting in an NPE. This is now also fixed.
Compare with the real clock implementation and here for where this would fail if we didn't return a function here.
| (swap! events #(cond-> % | ||
| (contains? % t) | ||
| (update t disj f))))] | ||
| (swap! events update t (fnil conj #{}) f) |
There was a problem hiding this comment.
could there be a race condition between the deref now on L194 and this swap?
There was a problem hiding this comment.
Only if there is a concurrent call to advance (which would be an odd thing to do in a test 😅). Even if, it wouldn't be an issue. It would only be one if we dereferenced now multiple times here which is what I prevented by pulling out the key calculation and save it in t for re-use. Makes sense?
The result deferred
dmay end up in an error state when(f)throws an exception or returns an error deferred. Now since the cancellation callback was only attached viad/chainfor side-effect (i.e. the resulting deferred is not returned), the dropped error detection would be triggered in this case. The fix is simply to also attach an error handler withd/on-realized.Furthermore, putting the result deferred in an error state would not have cancelled the timer because there was no error handler attached to it. This is now also fixed and the tests are extended to cover the cancellation API, too.
The latter uncovered an oversight in the mock clock implementation where the
inmethod didn't return a cancellation function, resulting in an NPE. This is now also fixed.Based on #245.